Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROF-10112] Simplify method names from actionview templates in profiler #3759

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 5, 2024

What does this PR do?

Rails's ActionView likes to dynamically generate method names with suffixed hashes/ids, resulting in methods with names such as _app_views_layouts_explore_html_haml__2304485752546535910_211320.

This makes these stacks not aggregate well, as well as being not-very-useful data.

This PR modifies the stack collector to detect such methods and simplify them by slicing the hash/id suffix.

Motivation:

Improve quality of aggregated flamegraph.

Additional Notes:

N/A

How to test the change?

Change includes test coverage. Also, all of our internal Rails test apps use templates, so by searching by methods with erb or haml we'll be able to validate they no longer end with the ids.

**What does this PR do?**

Rails's ActionView likes to dynamically generate method names with
suffixed hashes/ids, resulting in methods with names such as
`_app_views_layouts_explore_html_haml__2304485752546535910_211320`.

This makes these stacks not aggregate well, as well as being
not-very-useful data.

This PR modifies the stack collector to detect such methods and
simplify them by slicing the hash/id suffix.

**Motivation:**

Improve quality of aggregated flamegraph.

**Additional Notes:**

N/A

**How to test the change?**

Change includes test coverage. Also, all of our internal Rails test
apps use templates, so by searching by methods with `erb` or
`haml` we'll be able to validate they no longer end with the ids.
@ivoanjo ivoanjo requested a review from a team as a code owner July 5, 2024 08:21
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 5, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.91%. Comparing base (849c60c) to head (5c6441a).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3759      +/-   ##
==========================================
- Coverage   97.91%   97.91%   -0.01%     
==========================================
  Files        1241     1241              
  Lines       74632    74654      +22     
  Branches     3605     3605              
==========================================
+ Hits        73074    73095      +21     
- Misses       1558     1559       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@p-datadog p-datadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you expect any string without _app_ prefix to be altered by this logic? If this is specifically for Rails maybe check for this prefix?

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 8, 2024

Do you expect any string without app prefix to be altered by this logic? If this is specifically for Rails maybe check for this prefix?

This is a good point! I double-checked and the answer is actually that the prefix is the path to the template, .e.g _app_views_layouts_explore_html_haml__2304485752546535910_211320 is app/views/layouts/explore.html.haml.

And because it's a path, it's actually possible to change it. By using Rails.application.config.paths['app/views'].unshift(Rails.root.join('testcustom', 'views')) and then moving the file I got a frame named _testcustom_views_pages_about_html_erb for a test app I spun up.

So while I don't think it'll be very common that the template will come from outside the app folder, it can in rare situations, so I'm leaning towards keeping the more generic version.

I also don't think it would particularly be faster -- since we already have the file name filter (not ".rb") + most methods don't end with numbers, this should be enough to filter out almost all methods which are not templates.

@ivoanjo ivoanjo merged commit 6e1050a into master Jul 8, 2024
168 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10112-simplify-method-names branch July 8, 2024 09:03
@github-actions github-actions bot added this to the 2.2.0 milestone Jul 8, 2024
@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 8, 2024

I've gone ahead and merged the PR, but LMK if you think I should give another look at focusing only on the _app_ prefix 🙏 🙇

@p-datadog
Copy link
Member

My concern was that if someone has their methods named with numeric suffixes that happen to match Rails' generated method names that this PR is considering, they would have their methods unexpectedly renamed. But I wouldn't expect such naming to be very common either.

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 8, 2024

My concern was that if someone has their methods named with numeric suffixes that happen to match Rails' generated method names that this PR is considering, they would have their methods unexpectedly renamed. But I wouldn't expect such naming to be very common either.

Yeah, they'd need to be quite unlucky:

  • Ends with __ + number + _ + number
  • Filename is not .rb

Hopefully the current version is good enough; if someone ever triggers this by accident we'll have examples on how to tighten the pattern to not match the accidental one as well :)

ivoanjo added a commit that referenced this pull request Jul 10, 2024
**What does this PR do?**

This PR is a small fix on top of #3759. In that PR, we added code to
detect method names dynamically generated by Rails actionview templates,
slicing the parts of the method name that have random ids.

E.g. `_app_views_layouts_explore_html_haml__2304485752546535910_211320`
became `_app_views_layouts_explore_html_haml`.

When looking at one of our example applications, I realized that I
was seeing methods that ended with both `__number_number` as well as
`___number_number` (three vs two underscores), e.g.:

* `_app_views_articles_index_html_erb___2022809201779434309_12900`
* `_app_views_articles_index_html_erb__3816154855874711207_12920`

On closer inspection of the actionview template naming code in
https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389:

```ruby
    def method_name # :nodoc:
      @method_name ||= begin
        m = +"_#{identifier_method_name}__#{@identifier.hash}_#{__id__}"
        m.tr!("-", "_")
        m
      end
    end
```

... I realized that when `@identifier.hash` was a negative number,
it would be printed including the - sign, which would be replaced by
the `tr` into another `_`. (It's somewhat weird that they didn't just go
with `hash.abs` but yeah... >_>).

Thus, there was a 50-50 chance that methods end up with three
underscores, which would make them not merge together in the flamegraph.

This PR fixes this.

**Motivation:**

Make sure that these dynamically-generated methods merge together
properly in a flamegraph.

**Additional Notes:**

N/A

**How to test the change?**

This change includes test coverage.
@TonyCTHsu TonyCTHsu mentioned this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants